googleapis: DirectPath over Interconnect#12760
Conversation
|
The following changes are made outside of the design doc specifications or assuming few things:
|
| private static int c2pId = new Random().nextInt(); | ||
|
|
||
| private static synchronized BootstrapInfo getBootstrapInfo() | ||
| private static BootstrapInfo getBootstrapInfo(boolean isForcedXds) |
There was a problem hiding this comment.
The design didn't call this out. We need to decode if it is "first one wins" or we need two different bootstraps. I've added a comment on the design.
There was a problem hiding this comment.
Per Mark's thoughts on the design, they'll share the bootstrap file and the first one wins? So we're good with the current implementation for bootstrapInfo right?
We are stripping the force-xds parameter from the URI before passing it to the pool (InternalSharedXdsClientPoolProvider) and the target strings will be identical. So whichhever channel starts first will set the "winning" bootstrap configuration.
There was a problem hiding this comment.
The current implementation does not share and does not have a "first one wins" behavior. We use two different variables to store the caches... they are completely separate.
There was a problem hiding this comment.
Oh, I was assuming that finalBootstrapInfo is what we cared about which will set the bootstrap config in sharedXdsClientPoolProvider.
I'll update to use one single file here as well, that will obviously make it simpler having one lock.
There was a problem hiding this comment.
Oh, that's fair, although the changes here no longer served any purpose.
| if (part.equals("force-xds")) { | ||
| return true; | ||
| } | ||
| if (part.startsWith("force-xds=")) { |
There was a problem hiding this comment.
There is no value defined for this. Don't go making up your own, especially giving multiple aliases (false vs 0), as we need it to behave the same across languages. If it were necessary, we'd need to update the design so that all languages behaved the same. Since it doesn't seem necessary, delete this.
| if (!zone.isEmpty()) { | ||
| String nodeIdPrefix = isOnGcp ? "C2P-" : "C2P-non-gcp-"; | ||
| nodeBuilder.put("id", nodeIdPrefix + (c2pId & Integer.MAX_VALUE)); | ||
| if (!isForcedXds && !zone.isEmpty()) { |
There was a problem hiding this comment.
The zone is guaranteed to be empty when isForcedXds=true. And that is important, because we need to make sure not to look up the zone when forcing xds. Same for supportIpv6. We shouldn't have these conditions, because that should be handled before we get to this function. The isForcedXds argument should be removed. This should also return to being private, because the tests calling this directly don't actually test anything.
| private static int c2pId = new Random().nextInt(); | ||
|
|
||
| private static synchronized BootstrapInfo getBootstrapInfo() | ||
| private static BootstrapInfo getBootstrapInfo(boolean isForcedXds) |
There was a problem hiding this comment.
Oh, that's fair, although the changes here no longer served any purpose.
| Resource<Executor> executorResource, | ||
| NameResolver.Factory nameResolverFactory) { | ||
| this.executorResource = checkNotNull(executorResource, "executorResource"); | ||
| String query = targetUri.getRawQuery(); |
There was a problem hiding this comment.
Consider working with the query component as an instance of QueryParams rather than a string. That way you can parse it just once here, then read/modify it in both checkForceXds() and stripForceXds() without parsing it again.
3 participants
Implements go/directpath-interconnect-client
CC: @anicr7